-
Notifications
You must be signed in to change notification settings - Fork 1
Fix issues with in-place map/broadcast #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
- Coverage 63.40% 56.11% -7.30%
==========================================
Files 6 5 -1
Lines 276 278 +2
==========================================
- Hits 175 156 -19
- Misses 101 122 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the unaliasing logic makes sense, although it might be sensible to have a special code path for specific cases where map!(f, a, a)
is being called, since there you would probably simply loop a[i] = f(a[i])
without having to worry about zeroing out the storage.
This being said, looking at the code a bit more, I think this still is not handling zeropreserving
and nonzeropreserving
functions very well either right? Probably we should open an issue for that, since I would say that silent wrong results are way worse than throwing errors
I tried refactoring the code taking this and your comment about when to call
I think the logic right now is correct and safe but maybe not necessarily efficient, since it relies on allocating a prototypical unstored element and checking it is zero which may involve allocating a big array, in the case of sparse arrays of arrays. It is definitely something we should reevaluate and try to come up with a better design. I'll open an issue. (EDIT: Opened in #22.) |
Fixes #19, though I need to add tests before merging.
@lkdvos does this unaliasing logic look right to you? The tricky cases for me to think about are when the source and destination are overlapping slices of the same sparse array, I have some tests that check this logic works to fix both #19 and those cases but I can't tell if it is either being too conservative, or not conservative enough in some situation I haven't been able to devise...